Conversation
Previously, every operation in `socket` would release the GIL, allowing other threads to progress. This makes sense when using threads and blocking sockets, but when using non-blocking sockets (as done with asyncio) these operations return quickly. In the presence of background threads this can lead to the "convoy effect", where the GIL is acquired and held by the background thread every time the IO thread releases it, and then the IO thread blocks until it regains ownership of the GIL, causing a massive decrease in IO operations per second. One possible fix for this is to have IO-heavy threads release the GIL less frequently. In the case of asyncio, the following changes were needed: - Don't release the GIL for socket operations on non-blocking sockets. - Don't release the GIL for select (epoll, select, kqueue, ...) calls with a timeout of 0. With these changes, asyncio doesn't see as degraded of an output when a background thread holds the GIL.
|
|
||
|
|
||
| /* Conditionally release and restore the GIL. */ | ||
| #define _Py_BEGIN_ALLOW_THREADS_COND(cond) \ |
There was a problem hiding this comment.
I added some helper macros here, but am not sure if that's best practice. Happy to change this if others have a better suggestion.
| ev.data.fd = fd; | ||
| Py_BEGIN_ALLOW_THREADS | ||
| result = epoll_ctl(epfd, op, fd, &ev); | ||
| Py_END_ALLOW_THREADS |
There was a problem hiding this comment.
As far as I can tell epoll_ctl operations like this always complete quickly, so releasing the GIL here isn't really necessary.
| _Py_BEGIN_ALLOW_THREADS_COND(s->sock_timeout) | ||
| res = sock_func(s, data); | ||
| Py_END_ALLOW_THREADS | ||
| _Py_END_ALLOW_THREADS_COND |
There was a problem hiding this comment.
There are likely other socket operations where releasing the GIL could be avoided, but this gets the bulk of those done frequently in a loop (send/ recv, ...). socket.close() is also handled below.
|
This PR is stale because it has been open for 30 days with no activity. |
| Py_BEGIN_ALLOW_THREADS | ||
| _Py_BEGIN_ALLOW_THREADS_COND(s->sock_timeout) | ||
| res = SOCKETCLOSE(fd); | ||
| Py_END_ALLOW_THREADS | ||
| _Py_END_ALLOW_THREADS_COND |
There was a problem hiding this comment.
Don't change here. s->sock_timeout doesn't affect to close.
Unlike send/recv, close can not be called frequently more than creating socket. So this shouldn't be a big performance problem.
|
A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated. Once you have made the requested changes, please leave a comment on this pull request containing the phrase |
github-actions
Bot
removed
the
stale
|
This PR is stale because it has been open for 30 days with no activity. |
github-actions
Bot
removed
the
stale
serhiy-storchaka
changed the title
|
@codex review this. Please check whether it is really safe to stop releasing the GIL, including whether there are any cases on officially supported Python platforms where this operation can block for a significant amount of time. |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
Codex Review: Something went wrong. Try again later by commenting “@codex review”. ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
@copilot resolve the merge conflicts in this pull request |
5 participants
Previously, every operation in
socketwould release the GIL, allowingother threads to progress. This makes sense when using threads and
blocking sockets, but when using non-blocking sockets (as done with
asyncio) these operations return quickly. In the presence of background
threads this can lead to the "convoy effect", where the GIL is acquired
and held by the background thread every time the IO thread releases it,
and then the IO thread blocks until it regains ownership of the GIL,
causing a massive decrease in IO operations per second.
One possible fix for this is to have IO-heavy threads release the GIL
less frequently. In the case of asyncio, the following changes were
needed:
with a timeout of 0.
See the corresponding issue at https://bugs.python.org/issue45819
for more information.
https://bugs.python.org/issue45819